Skip to content

Add support for code generation for List<X> types#2402

Open
yinzara wants to merge 1 commit into
npgsql:mainfrom
yinzara:feature/2401
Open

Add support for code generation for List<X> types#2402
yinzara wants to merge 1 commit into
npgsql:mainfrom
yinzara:feature/2401

Conversation

@yinzara

@yinzara yinzara commented Jun 10, 2022

Copy link
Copy Markdown

Fixes #2401

@yinzara

yinzara commented Jun 10, 2022

Copy link
Copy Markdown
Author

I also added a few additional tests to verify that array type columns are being generated properly as well as inserting default and HasData as I could not find any

@roji

roji commented Jun 10, 2022

Copy link
Copy Markdown
Member

This seems like it adds support for new[] { "1", "2" }.ToList(), but what we want is new List<string> { "1", "2" }, no?

@yinzara

yinzara commented Jun 10, 2022

Copy link
Copy Markdown
Author

lol I agree yes that's exactly what I did.

Let me fix that. I didn't realize there was a ListInit expresion function. I was having trouble finding it but:
https://docs.microsoft.com/en-us/dotnet/api/system.linq.expressions.expression.listinit?view=net-6.0

there it is :)

@yinzara

yinzara commented Jun 10, 2022

Copy link
Copy Markdown
Author

Looks like there's a problem with that. When I use the following code:

public override Expression GenerateCodeLiteral(object value)
    {
        var values = (System.Collections.IList)value;
        List<Expression> elements = new(values.Count);
        var generated = true;
        foreach (var element in values)
        {
            if (generated)
            {
                try
                {
                    elements.Add(ElementMapping.GenerateCodeLiteral(element)); // attempt to convert if required
                    continue;
                }
                catch (NotSupportedException)
                {
                    generated = false; // if we can't generate one element, we probably can't generate any
                }
            }
            elements.Add(Expression.Constant(element));
        }
        return Expression.ListInit(Expression.New(typeof(List<>).MakeGenericType(ElementMapping.ClrType)), elements);
    }

I get the error:

The literal expression 'new List`1() {Void Add(Int32)(1), Void Add(Int32)(2), Void Add(Int32)(3)}' for 'List<int>' cannot be parsed. Only simple constructor calls and factory methods are supported.

so it looks like we might have to stick with the array initializer calling ToList() on it.

@roji

roji commented Jun 11, 2022

Copy link
Copy Markdown
Member

IIRC this is why I originally implemented array literals but not List...

I think we should simply add support for this on the EF side - it shouldn't be too hard. Then we'd be able to generate the right thing here.

@yinzara

yinzara commented Jun 11, 2022

Copy link
Copy Markdown
Author

I certainly don't mind doing that either. If you have a hint of where you think it might be it would be exceedingly helpful.

@roji

roji commented Jun 11, 2022

Copy link
Copy Markdown
Member

@yinzara

yinzara commented Jun 13, 2022

Copy link
Copy Markdown
Author

Alrighty. I think what I've done should fix this issue. In fact, it might just fix it without the code in this PR since I've now added support for List literals in EF Core but I'm going to test locally and see what the outcome is.

@yinzara

yinzara commented Jun 13, 2022

Copy link
Copy Markdown
Author

Yup.... this PR isn't even necessary once the other PR to EFCore is merged.

All my test cases I wrote as part of this PR now pass without the code changes to NgpgsqlArrayTypeMapping or NgpgsqlArrayListTypeMapping if I change my local NuGet to point to the compiled source from the other PR.

Would you like to me to just close this PR completely or leave it open with just the tests pending the other PR being merged so that we have additional tests for these cases?

yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
@roji

roji commented Jun 13, 2022

Copy link
Copy Markdown
Member

Thanks @yinzara, yeah - having tests here would be a good thing. Once the support is merged EF-side, I'll sync here to use the latest version and we can merge tests.

yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
ghost pushed a commit to dotnet/efcore that referenced this pull request Jun 27, 2022
* Add support for CSharpHelper for List literals

Fixes #19274

Also relates to npgsql/efcore.pg#2402

* Fixes from review comments
@roji

roji commented Jul 9, 2022

Copy link
Copy Markdown
Member

@yinzara the main branch of EFCore.PG has been synced to the latest EF Core daily build, which contains your List support from dotnet/efcore#28212. So you can add tests now.

@roji

roji commented Aug 1, 2022

Copy link
Copy Markdown
Member

@yinzara are you planning to add the appropriate tests here?

@roji roji force-pushed the main branch 8 times, most recently from 3bd8fcb to 79fae3a Compare February 16, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support HasData for List<X> type fields

2 participants